Reproduce game weak data with a PCH - #130

Report for GMSJ01 (9b62e09 - d83decd)

📈 Matched code: 37.73% (+0.57%, +20624 bytes)
📈 Matched data: 55.11% (+6.02%, +38576 bytes)

Follow-up commit a7e5756 centralizes the common game compiler profile in cflags_game: -opt all,nostrength, -inline deferred, and -prefix SMS.mch. The PCH build and the three evidence-backed no-PCH object exceptions retain explicit flag lists; THPPlayer remains on its separate profile. Verified with a full build (mario.dol: OK), ninja baseline/changes_all (no regressions), Python syntax validation, and git diff --check.

Edited: this comment predated the rebase. The commit is now ca3368df, the figures below were measured against the old base, and the JKRArchive follow-up described here turned out to be unnecessary. Corrected inline; the full write-up now lives in the PR description under "MapObjGeneral vtable ownership".

Resolves the "Known edge case" that was in the description — MapObjInit no longer needs its stand-in literals.

Root cause. Retail's MapObjInit.o owns __vt__14TMapObjGeneral, the getFlushTime weak body, and the @32@__dt__14TMapObjGeneral adjustor thunk. We were emitting all three in MapObjGeneral.o. They travel together, and the missing .data is exactly what dummy/dummy2/dummy3/dummy4 were standing in for.

MWCC picks the owning TU by key function: the first non-inline virtual in declaration order. Ours was loadAfter (MapObjGeneral.cpp); retail's must have been initMapObj (MapObjInit.cpp). initMapObj overrides a TMapObjBase virtual, so its slot is fixed by the base and promoting its declaration cannot change the vtable layout — and doesn't, the table is still exactly 0x1dc.

Second fix. getFlushTime had an empty body { } — 4 bytes, and the source of the return value expected warning in the build log. Retail is 8 bytes:

lwz r3, TMapObjGeneral::mNormalFlushTime@sda21
blr

mNormalFlushTime already existed as a static, so the real body mirrors getLivingTime() directly above it. 100% match.

MapObjInit data goes 0.389% → 44.203%; the remainder is genuine unreconstructed content, not padding. For the whole-PR figures, see the table in the description — the ones originally quoted here were against a base that has since been superseded by the rebase.

Codebase sweep. I checked for the same bug elsewhere by resolving every __vt__ owner in mario.MAP against the TU defining that class's first non-inline virtual. 624 classes resolved of 1508 vtables, and TMapObjGeneral is the only mismatch. An earlier version of this comment reported JKRArchive as a second one and said I would send it separately; that was wrong — it had already been fixed upstream in c6a3188, which even documents the same technique:

// NOTE: has to be defined not first to get the vtable
// to emit to the correct TU
virtual ~JKRArchive();

My sweep had been reading the pre-sweep header on this branch. No follow-up PR is coming, and none is needed.

Rebased onto current main and refreshed the description. Dropping the WIP prefix — this is ready for review.

Three things changed since the last look:

CI. The Validate symbol order failure was not from this PR. The workflow picks its file list with a two-dot git diff "$base" HEAD, so a branch that is behind main gets every file main has touched added to the list and then validated in its stale copy — 105 units checked, 31 failed, none of them files this PR modifies. Rebasing alone took it to 10 units, all passing. Filed separately as #151 with a suggested git merge-base fix, since it will keep biting other PRs.

Description. The Results table was measured against a superseded baseline and the "Known edge case" section described the MapObjInit stand-ins, which ca3368df fixed. Both rewritten against the current base.

Re-audited the PCH selection. main has changed 101 source files since the original audit, and 11 of the 40 PCH units were among them, so the evidence behind the list needed redoing. Three-way controlled experiment, toggling only the PCH:

Per-unit across the 40: 23 improve, 17 no effect, 0 regress. Across the 696 non-PCH units: none would gain, and exactly three regress under the global build — RumbleData, spcinterp, gd-reinit-gx, the same three the original audit excluded. So the boundary is unchanged and the selection needs no edits.

The 17 inert units are kept deliberately: seven are still at 0% code and 0% data, so there is nothing for the PCH to act on yet, and two are already at 100% data. Happy to trim them to 23 if you would rather the list carry only units with present-day evidence — it makes no difference to the build either way.